Parcel 2: Simple working JS packager #2239
Conversation
| nodeFromDep({...dep, sourcePath: file.filePath}) | ||
| ); | ||
| let depNodes = assetNode.value.dependencies.map(dep => { | ||
| dep.sourcePath = file.filePath; |
There was a problem hiding this comment.
To discuss: do we even need this field on a dependency? An Asset node points to a Dependency in the graph, so we should be able to infer the file the dep came from based on its location in the graph.
There was a problem hiding this comment.
It's used to generate the id when creating the node (
). I like the idea of the value being the only input to create a node, but I suppose we could consider removing it if it really isn't useful and is taking up space.There was a problem hiding this comment.
Yeah I saw that. But we could pass the source path as a second argument to nodeFromDep to create the id... Perhaps it is useful information to store on a dep. I'm not sure.
| @@ -174,6 +174,7 @@ export default class Parcel { | |||
|
|
|||
| let file = {filePath: resolvedPath}; | |||
| if (signal && !signal.aborted) { | |||
| dep.resolvedPath = resolvedPath; | |||
There was a problem hiding this comment.
Not sure about mutating the dep here. Also we might not want to store this in the dep anyway - we could just figure it out based on where it is in the graph. That wouldn't work if we want to cache the resolution though, so perhaps we do want it in the end. Either way, it cannot be on the dep when it is initially created - it will need to be filled in later post-resolution.
There was a problem hiding this comment.
If we are going to add this field to the dep, it might make more sense to do it in the AssetGraph's updateDependency function.
That wouldn't work if we want to cache the resolution though
Are talking about the parcel cache or like the in memory "cache" on the v1 Resolver? Caching in memory is pretty much already taken care of by the way the graph is built. If a file is transformed a second time and finds some of the same dependencies it won't try to resolve them again. As for the parcel cache, I'm wondering if we want to cache resolved paths, at least to begin with. Seems like cache invalidation for that could get hairy.
There was a problem hiding this comment.
I'm talking about the filesystem cache here. In Parcel 1 we do cache the resolved paths, and it increases performance very significantly.
| let deps = {}; | ||
|
|
||
| for (let dep of asset.dependencies) { | ||
| let resolvedAsset = bundle.assets.find( |
There was a problem hiding this comment.
This isn't a great way to find the resolved ids for deps. It will be slower than necessary for sure. Perhaps another reason to have a bundle contain a Graph instead of a flat list of assets.
There was a problem hiding this comment.
For sure, I can start looking into that. We'll have to be able serialize/deserialize graphs since the packagers will be running in workers. Do you think that could possibly end up making it not worth it performance wise?
| nodeFromDep({...dep, sourcePath: file.filePath}) | ||
| ); | ||
| let depNodes = assetNode.value.dependencies.map(dep => { | ||
| dep.sourcePath = file.filePath; |
There was a problem hiding this comment.
It's used to generate the id when creating the node (
). I like the idea of the value being the only input to create a node, but I suppose we could consider removing it if it really isn't useful and is taking up space.| let deps = {}; | ||
|
|
||
| for (let dep of asset.dependencies) { | ||
| let resolvedAsset = bundle.assets.find( |
There was a problem hiding this comment.
For sure, I can start looking into that. We'll have to be able serialize/deserialize graphs since the packagers will be running in workers. Do you think that could possibly end up making it not worth it performance wise?
| @@ -174,6 +174,7 @@ export default class Parcel { | |||
|
|
|||
| let file = {filePath: resolvedPath}; | |||
| if (signal && !signal.aborted) { | |||
| dep.resolvedPath = resolvedPath; | |||
There was a problem hiding this comment.
If we are going to add this field to the dep, it might make more sense to do it in the AssetGraph's updateDependency function.
That wouldn't work if we want to cache the resolution though
Are talking about the parcel cache or like the in memory "cache" on the v1 Resolver? Caching in memory is pretty much already taken care of by the way the graph is built. If a file is transformed a second time and finds some of the same dependencies it won't try to resolve them again. As for the parcel cache, I'm wondering if we want to cache resolved paths, at least to begin with. Seems like cache invalidation for that could get hairy.
Wraps all assets in functions.